Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In @.github/workflows/frontier_amd/submit.sh:
- Around line 31-32: The job_slug currently built by job_slug and used for
output_file omits the shard, causing name collisions; update the job_slug
generation (the job_slug variable and any references to output_file) to append
the shard identifier (formatting the shard like "-{shard}" and replacing "/"
with "-of-" for values like "1/2") so each shard produces a unique slug; also
update the script usage message (the usage text near the top that lists
parameters) to document the interface and shard parameters, and update the
workflow steps that read and upload artifacts (the cat command that reads
test-${matrix.device}-${matrix.interface}.out and the artifact name) to include
the same shard suffix formatting so artifact names and printed output match the
new job_slug convention.

In @.github/workflows/test.yml:
- Around line 265-274: Update the Print Logs and Archive Logs steps so the
logfile and artifact name include the shard-aware slug used by the submit
scripts (instead of assuming test-${{ matrix.device }}-${{ matrix.interface
}}.out). Locate the "Print Logs" and "Archive Logs" steps and change the
referenced filename and artifact name to incorporate the sanitized job
slug/shard token produced by the submit scripts (the same slug that replaces "/"
with a safe separator such as "-of-"); ensure the workflow expression that
builds the filename and the artifact "name" use that sanitized slug so filenames
and artifact names remain unique and valid across sharded jobs.

---

Duplicate comments:
In @.github/workflows/frontier/submit.sh:
- Around line 31-32: job_slug and output_file are colliding for parallel shards
because they only use basename("$1") with $2 and $3; update the job_slug
generation (and derived output_file) to include an additional unique shard
identifier (for example a shard index/ID passed as another script argument or a
runtime value like the process/array task id) so each shard produces a distinct
job_slug and output_file; change the construction that sets job_slug and the
assignment of output_file to append that unique identifier.

---

Nitpick comments:
In @.github/workflows/frontier_amd/submit.sh:
- Around line 8-9: The usage() function's message is outdated and only mentions
two arguments; update the echo in usage() to document all supported positional
params ($1 script, $2 device (cpu|gpu), $3 interface, $4 shard) and any defaults
or optional markers (e.g., "[interface]" "[shard]") so callers see the full
signature; edit the echo inside usage() to a single clear line listing script,
device, interface, and shard and optional/default semantics.

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name=".github/workflows/phoenix/test.sh">

<violation number="1" location=".github/workflows/phoenix/test.sh:5">
P2: The `./mfc.sh clean` exit status is not checked. If the clean fails, the script continues and may build/test against stale or corrupted artifacts, defeating the purpose of this proactive cleanup and causing hard-to-diagnose failures.</violation>
</file>

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@toolchain/mfc/test/test.py`:
- Around line 217-224: When abort_tests.is_set() causes test() to raise
MFCException the existing cleanup that writes/removes failed_uuids.txt (the
failed_uuids_path handling around failed_tests, open(...), os.remove(...)) is
skipped; modify the exception/exit path to always attempt to remove stale
failed_uuids_path and wrap file I/O (open and os.remove) in try/except catching
OSError (or Exception) so I/O errors are logged but do not replace the real exit
code—i.e., in the MFCException handler and/or finally block ensure you try to
delete failed_uuids_path if it exists and handle/log any OSError from
open()/os.remove() instead of letting it propagate.

---

Nitpick comments:
In `@toolchain/mfc/test/test.py`:
- Around line 102-108: The compound validation in the ARG("shard") block is
correct but hard to read; refactor the conditional inside the if ARG("shard") is
not None: block by splitting the long compound condition into explicit guard
checks: first split = ARG("shard").split("/") and verify length == 2, then check
that both parts are digits (using parts[0].isdigit() and parts[1].isdigit()),
then parse shard_idx = int(parts[0]) and shard_count = int(parts[1]) and
validate shard_count >= 1 and 1 <= shard_idx <= shard_count; on any failure
raise MFCException with the same message, then compute skipped_cases and cases
using shard_idx and shard_count as before.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.github/workflows/test.yml:
- Around line 146-166: The workflow is triggering shellcheck/ actionlint SC2046
on unquoted command substitution for nproc; update both invocations of -j
$(nproc) in the test run commands (the two calls to "/bin/bash mfc.sh test -v
... -j $(nproc) ..." and the retry call) to quote the substitution as -j
"$(nproc)"; leave $TEST_ALL, $TEST_PCT and the intentionally unquoted $FAILED
as-is and ensure the surrounding logic using TEST_EXIT, failed_uuids.txt and the
retry branch remains unchanged.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@toolchain/mfc/test/test.py`:
- Around line 193-196: The unguarded os.remove(failed_uuids_path) can raise
OSError and mask the original MFCException on the abort path; wrap the call to
os.remove (referring to failed_uuids_path) in a try/except OSError that either
logs the exception or quietly ignores it (consistent with the I/O block pattern
used elsewhere) so the original MFCException from the abort flow is preserved.

---

Duplicate comments:
In @.github/workflows/test.yml:
- Line 31: The quoting issue for the CPU count in the workflow step has already
been fixed; no code change is required—leave the step calling the script as
"./mfc.sh format -j \"$(nproc)\"" in the GitHub Actions workflow to preserve
correct quoting and shell expansion.

In `@toolchain/mfc/test/test.py`:
- Around line 222-229: The code writing/removing failed_uuids_path (using
failed_uuids_path, failed_tests, open(), os.remove()) is unguarded and can raise
I/O errors that would replace the intended exit(nFAIL); wrap the write and the
remove branches in try/except blocks that catch Exception, log or print the
error with context (including the path and exception), and ensure the script
still calls exit(nFAIL) after handling the I/O error so the test failure status
is preserved.

---

Nitpick comments:
In @.github/workflows/test.yml:
- Around line 277-282: The artifact name for the "Archive Logs" step currently
uses logs-${{ strategy.job-index }}-${{ matrix.device }}-${{ matrix.interface
}}, which omits the shard; update the Upload Artifact step (name: Archive Logs,
uses: actions/upload-artifact@v4) to append the sanitized shard value from
matrix.shard (use the replace expression to substitute '/' with '-' e.g.
replace(matrix.shard, '/', '-')) so the artifact name becomes logs-${{
strategy.job-index }}-${{ matrix.device }}-${{ matrix.interface }}-<sanitized
shard>, ensuring shard information is visible and safe for GitHub artifact
naming.
- Around line 146-166: The assignment to FAILED currently uses a useless cat
pipeline ("FAILED=$(cat tests/failed_uuids.txt | tr '\n' ' ')") which spawns an
extra process; change it to read the file directly into tr or use shell builtins
to avoid forking (for example use input redirection with tr like "FAILED=$(tr
'\n' ' ' < tests/failed_uuids.txt)" or use readarray/mapfile to populate and
join), leaving the surrounding logic (NUM_FAILED, retry invocation of mfc.sh,
and TEST_EXIT handling) untouched and still referencing tests/failed_uuids.txt
and the FAILED variable.

In `@toolchain/mfc/test/test.py`:
- Line 104: The guard currently uses str.isdigit() on elements of parts in the
conditional (the expression containing "len(parts) != 2 or not all(p.isdigit()
for p in parts) ..."), which allows non-ASCII numeric characters that int()
can't parse; update the predicate to use str.isdecimal() instead (i.e., replace
the all(p.isdigit() for p in parts) check with all(p.isdecimal() for p in
parts)) so only ASCII digits [0-9] are accepted before calling int() on parts[0]
and parts[1].